-
Notifications
You must be signed in to change notification settings - Fork 207
[Set Window Rect] Change parameter requirement from Number to Integer and range to "maximum safe integer" #1909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
index.html
Outdated
@@ -4058,11 +4058,11 @@ <h4><dfn>Set Window Rect</dfn></h4> | |||
<li><p>If <var>y</var> is <a>undefined</a>, let <var>y</var> be null. | |||
|
|||
<li><p>If <var>width</var> or <var>height</var> is neither null, nor | |||
a <a>Number</a> from 0 to 2<sup>31</sup> − 1, return <a>error</a> | |||
an integer from 0 to 2<sup>31</sup> − 1, return <a>error</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find. But lets take one more thing into account. In WebDriver BiDi we actually use a range from 0 to maximum safe integer. We probably should do the same here as well even though those high values won't be able to get applied. The same applies for x
and y
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In WebDriver BiDi we actually use a range from 0 to maximum safe integer.
Can you show a reference of this case in BiDi? I cannot find it anywhere.
I only see usage of "maximum safe integer" in Add Cookie/Timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same applies for
x
andy
.
Also it seems x
and y
can be negative? How would we phrase it if upper bound is maximum safe integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CDDL definition uses https://w3c.github.io/webdriver-bidi/#cddl-type-js-uint. That's basically the maximum safe value. There is also js-int
for the full range. In WebDriver classic see https://w3c.github.io/webdriver/#dfn-maximum-safe-integer.
web-platform-tests/wpt#53421 (comment) Can you also review this one after? Thanks!! |
I think this might actually be correct and BiDi wrong? It seems implausible that any implementation actually allows window dimensions larger than 32 bits (real life screen sizes typically fit into 13 bits). Also with high DPI screens it's possible that the width can be set to a non-integer number of CSS pixels (e.g. for a 2x DPI display a width of 800.5 CSS pixels would be 1601 device pixels). |
Yeah this is true. I also had similar doubt when seeing the test.. |
Do you think we should change the test instead of the spec? @jgraham |
The spec says Number, which is "double-precision 64-bit binary format IEEE 754 value". But the wpt test below clearly requires integer.
https://github.com/web-platform-tests/wpt/blob/b281d07f3ead48995b9a2e94259d292e22cd5dd9/webdriver/tests/classic/set_window_rect/set.py#L41-L49
This PR adds definition of minimum safe integer and change the valid range of parameter.
Preview | Diff